Whitelist rustc dependencies #48456

mark-i-m · 2018-02-23T02:00:54Z

As per https://internals.rust-lang.org/t/rustc-dependency-policy/6537/8?u=nikomatsakis

cc @alexcrichton @nikomatsakis

rust-highfive · 2018-02-23T02:01:06Z

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

mark-i-m · 2018-02-23T02:06:44Z

Hopefully, travis fails because I haven't added any files to the whitelist yet! 😛

mark-i-m · 2018-02-23T02:39:02Z

Hmmm... It seems that it only runs stage0 tidy? How do I get my tidy to work...

nikomatsakis · 2018-02-23T21:57:29Z

@mark-i-m travis...isn't failing?

nikomatsakis

Any idea why travis seems happy?

nikomatsakis · 2018-02-23T21:58:05Z

src/tools/tidy/src/deps.rs

        saw_dir = true;
        let dir = t!(dir);

        // skip our exceptions
-        for exception in EXCEPTIONS {
-            if dir.path()
+        if EXCEPTIONS.iter().any(|exception| {


nikomatsakis · 2018-02-23T21:59:00Z

src/tools/tidy/src/deps.rs

+             }| {
+                WHITELIST
+                    .iter()
+                    .all(|&(wname, wversion)| name != wname || version != wversion)


seems like it would be nice if we could report what needs to be added to the whitelist

mark-i-m · 2018-02-24T00:03:40Z

Ok, it should fail now 🤞

mark-i-m · 2018-02-24T00:04:13Z

MAKE IT FAILgit statusgit status

Lol, that's what I get for using !!!! in a git commit message...

mark-i-m · 2018-02-24T00:26:03Z

Hmm... it looks like paths are incorrect...

mark-i-m · 2018-02-24T00:36:56Z

It looks like a spurious failure... I just want to know what path the Cargo.toml for the argument of tidy is at.

mark-i-m · 2018-02-25T03:18:01Z

So currently, I'm having path failures. I'm looking for a Cargo.toml in /checkout/src/Cargo.toml and there isn't one... does anyone know the correct place to find it?

[00:05:02] * 534 error codes
[00:05:02] * highest error code: E0908
[00:05:02] * 175 features
[00:05:02] Getting metadata from "/checkout/src/Cargo.toml"
[00:05:02] thread 'main' panicked at 'Unable to run `cargo metadata`: Os { code: 2, kind: NotFound, message: "No such file or directory" }', libcore/result.rs:945:5
[00:05:02] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:05:02] 
[00:05:02] 
[00:05:02] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "--no-vendor" "--quiet"
[00:05:02] expected success, got: exit code: 101
[00:05:02] 
[00:05:02] 
[00:05:02] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:05:02] Build completed unsuccessfully in 0:01:57
[00:05:02] Makefile:74: recipe for target 'tidy' failed
[00:05:02] make: *** [tidy] Error 1

alexcrichton · 2018-02-26T16:14:47Z

@mark-i-m that looks like it's perhaps failing to spawn cargo rather than failing to find Cargo.toml? By default I believe cargo isn't in PATH so rustbuild may need to pass the path to cargo down to the tidy script.

mark-i-m · 2018-02-26T17:25:51Z

Ok, it seems to be working 🎉

There are currently two variables which need to be hardcoded into tidy, though:

The set of crates whose dependencies we want to check (currently those corresponding to the compiler)
The whitelist itself

So we have a choice:

It is conceivable that we want to have different whitelists for rustc, libstd, tooling, books, etc... In that case, maybe those variables shouldn't be hardcoded into tidy but passed in from somewhere else.
The simple solution for now is to just hardcode into tidy the whitelist for rustc and always check the 'librustc*' and 'libsyntax*' crates.

alexcrichton

Looking good! I think it's fine to hardcode the crates to check and whitelist in the code itself. For the crates to check you can probably just probe the entire dependency graph for dependencies from crates.io starting from rustc and rustc_trans.

alexcrichton · 2018-02-26T20:56:29Z

src/tools/tidy/src/deps.rs

+    #[allow(dead_code)] manifest_path: String,
+}
+
+// Not used, but needed to not confuse serde :P


Hm if these aren't used, are they needed? I think serde should ignore unused fields by default?

Hmm... When I tried that it was giving me deserialization errors. I will take another look.

alexcrichton · 2018-02-26T20:57:59Z

src/tools/tidy/src/deps.rs

+
+// Whitelist of crates rustc is allowed to depend on. Avoid adding to the list if possible.
+static WHITELIST: &'static [(&'static str, &'static str)] = &[
+//    ("advapi32-sys", "0.2.0"),


I think it's probably ok to only check the name of the dependency and not worry about the version for now, upgrading across major versions and such tends to not bring in too large of a change at least!

I argue that it's important for security purposes to whitelist exact versions and to vet those versions before before upgrading in any way.

Oh sure yeah, but I think we'll want to consider this further down the road. If we were to lock down all the versions of everything it'd cause I think picking up deps from crates.io to be too painful (or otherwise routine updates)

I think this is part of the downside of having a super-awesome dependency system: it becomes easy to have too many dependencies. Argue that that makes code quality, security, and build times all worse.

Sure but I think it's also best to do this incrementally rather than all at once, can this switch to just verifying the names through a whitelist?

I can if you would like, but I would really like not to. I feel like adding a bit of friction to adding and updating dependencies would be healthy; in this case, you would need to update WHITELIST. We can later have an RFC to potentially add more restrictions to PRs that change WHITELIST... I see that as an incremental approach as well (we add a mechanism, then add policy through an RFC).

mark-i-m · 2018-02-27T05:47:02Z

@alexcrichton Ok, so I am now only checking the dependencies of rustc and rustc_trans. (I will uncomment the whitelist when the build fails). I designed the Crate type so that we can easily turn on/off version checking, as you choose.

alexcrichton · 2018-02-27T12:15:21Z

src/tools/tidy/src/deps.rs

+    resolve: Resolve,
+
+    // Not used, but needed to not confuse serde :P
+    #[allow(dead_code)] packages: Vec<Package>,


Could this get removed? If not, how come?

alexcrichton · 2018-02-27T12:15:57Z

src/tools/tidy/src/deps.rs

+        unapproved.append(&mut bad);
+    }
+
+    // Remove duplicates


Perhaps this could be passed around as &mut BTreeSet<Crate<'a>>?

alexcrichton · 2018-02-27T12:16:53Z

Can this please remove the version checking as well? That happens anyway when Cargo.lock changes and is reviewed. The purpose of this feature is to prevent accidental inclusion of extra crates from crates.io or otherwise alert ourselves to new dependencies.

mark-i-m · 2018-02-27T18:03:35Z

@alexcrichton done 👍

Let me know what you think :)

mark-i-m · 2018-02-27T18:04:02Z

I will uncomment the whitelist itself when the build fails...

mark-i-m · 2018-02-27T18:48:03Z

Ok, I uncommented the whitelist.

alexcrichton · 2018-02-27T20:37:00Z

Looks good to me!

I think a few more deps may need to be added though?

mark-i-m · 2018-02-27T21:56:46Z

Oh, hmm... I wonder how that happened 😖

Fixed 👍

alexcrichton · 2018-02-28T00:13:11Z

Looks like another crate is missing? Although I don't know how core got in there...

mark-i-m · 2018-02-28T00:34:28Z

Hmm... different versions of a crate may have different dependencies... I think this should be fixed now.

Once again, I will wait for build failure and then enable the whitelist.

mark-i-m · 2018-03-05T20:45:00Z

@alexcrichton Hmm... I tried rebasing on the latest master, but I didn't get any conflicts locally. Do you know what's going on here?

alexcrichton · 2018-03-05T20:54:44Z

Ah yeah sometimes bors is not so good at merges...

@bors: r+

bors · 2018-03-05T20:54:45Z

📌 Commit e5d2920 has been approved by alexcrichton

bors · 2018-03-06T00:44:28Z

⌛ Testing commit e5d2920 with merge 1733a61...

@alexcrichton

Whitelist rustc dependencies As per https://internals.rust-lang.org/t/rustc-dependency-policy/6537/8?u=nikomatsakis cc @alexcrichton @nikomatsakis

bors · 2018-03-06T03:45:06Z

💔 Test failed - status-appveyor

alexcrichton · 2018-03-06T03:46:40Z

All dist jobs passed, merging manually

rust-highfive assigned alexcrichton Feb 23, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 23, 2018

nikomatsakis reviewed Feb 23, 2018

View reviewed changes

kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 25, 2018

alexcrichton reviewed Feb 26, 2018

View reviewed changes

alexcrichton reviewed Feb 27, 2018

View reviewed changes

mark-i-m changed the title ~~[WIP] Whitelist rustc dependencies~~ Whitelist rustc dependencies Feb 27, 2018

mark-i-m force-pushed the whitelist branch from 878e4d5 to 24e929d Compare February 28, 2018 00:47

mark-i-m and others added 16 commits March 5, 2018 14:43

Start adding a whitelist for rustc dependencies

3ee4104

Comments

d626218

MAKE IT FAILgit statusgit status

3570b9d

Trying to get paths right...

6e016c5

Get the path to cargo from rustbuild

b9b1c37

Only check the whitelist for some crates

50876d1

Fix alexcrichton's comments

e3a374a

uncomment whitelist

f89abd6

Remove spurious whitespace

489f2f1

Add a few missing deps

0fc5dae

different versions may have different deps

18f0533

Don't check in-tree deps

1fca9e0

enable whitelist

6180a3f

Do check the deps of path deps

bb01f31

Uncomment whitelist

1423da8

Add ena to whitelist

e5d2920

mark-i-m force-pushed the whitelist branch from 2bc27d3 to e5d2920 Compare March 5, 2018 20:44

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 5, 2018

bors added a commit that referenced this pull request Mar 6, 2018

Auto merge of #48456 - mark-i-m:whitelist, r=alexcrichton

1733a61

Whitelist rustc dependencies As per https://internals.rust-lang.org/t/rustc-dependency-policy/6537/8?u=nikomatsakis cc @alexcrichton @nikomatsakis

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 6, 2018

alexcrichton merged commit e5d2920 into rust-lang:master Mar 6, 2018

mark-i-m deleted the whitelist branch November 14, 2018 16:40

Whitelist rustc dependencies #48456

Whitelist rustc dependencies #48456

Conversation

mark-i-m commented Feb 23, 2018

rust-highfive commented Feb 23, 2018

mark-i-m commented Feb 23, 2018

mark-i-m commented Feb 23, 2018

nikomatsakis commented Feb 23, 2018

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark-i-m commented Feb 24, 2018

mark-i-m commented Feb 24, 2018

mark-i-m commented Feb 24, 2018

mark-i-m commented Feb 24, 2018

mark-i-m commented Feb 25, 2018

alexcrichton commented Feb 26, 2018

mark-i-m commented Feb 26, 2018

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark-i-m commented Feb 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Feb 27, 2018

mark-i-m commented Feb 27, 2018

mark-i-m commented Feb 27, 2018

mark-i-m commented Feb 27, 2018

alexcrichton commented Feb 27, 2018

mark-i-m commented Feb 27, 2018

alexcrichton commented Feb 28, 2018

mark-i-m commented Feb 28, 2018

mark-i-m commented Mar 5, 2018

alexcrichton commented Mar 5, 2018

bors commented Mar 5, 2018

bors commented Mar 6, 2018

bors commented Mar 6, 2018

alexcrichton commented Mar 6, 2018